Skip to content

RFC #48: Split 'assert_precondition' into optional and non-optional variants #48

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Apr 3, 2020
Merged
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 59 additions & 0 deletions rfcs/assert_precondition_rename.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
# RFC 43: Split 'assert\_precondition' into optional and non-optional variants

## Summary

Split the `assert_precondition` function into `assert_implements_optional` and
`assert_implements`. The `assert_implements_optional` function will behave as
`assert_precondition` does today; a failed assert will record a
'PRECONDITION\_FAILED' status. The `assert_implements` function behaves
similarly, but records a 'FAIL' status if the assert fails.

Reuse the 'PRECONDITION\_FAILED' status rather than renaming it due to the
technical difficulties of getting mozlog updated.

## Details

### Background

The `assert_precondition` function and associated status
('PRECONDITION\_FAILED') were added in [RFC #16](assert_precondition.md).
However there has been confusion over whether the function is meant for spec
features that are explicitly marked OPTIONAL (e.g. its usage in
`html/semantics/embedded-content/the-video-element/resize-during-playback.html`),
or whether it is meant as an early-out when a particular spec or feature is not
implemented by a browser (e.g. its usage in `portals/`).

The difference matters here due to the 'PRECONDITION\_FAILED' status, which has
been interpreted by some users as a 'non-failing' status - which only makes
sense if the failure is for truly OPTIONAL behavior.

### Proposal

Rename the `assert_precondition` function to `assert_implements_optional`, with
the same 'PRECONDITION\_FAILED' status produced, and add a new function
`assert_implements` that produces a 'FAIL' status if the input is not
[truthy](https://developer.mozilla.org/en-US/docs/Glossary/Truthy). That is:

> __assert_implements(condition, description)__
>
> Concludes the test with a `FAIL` status if `condition` is not
> [truthy](https://developer.mozilla.org/en-US/docs/Glossary/Truthy).
> Used to avoid running unnecessary code or subtests for a feature that is not
> implemented by the running browser.

> __assert_implements_optional(condition, description)__
>
> Concludes the test with a `PRECONDITION_FAILED` status if `condition` is not truthy.
> Used for skipping tests or subtests for OPTIONAL features that that would otherwise
> fail due to lack of support for the feature.

The 'PRECONDITION\_FAILED' status is not being renamed, as changing the logging
infrastructure (mozlog, etc) is logistically difficult.

## Risks

* As with any change in testharness.js, downstream embedders may have to spend
effort to adapt to the deprecation and rename. That said, there are currently
zero uses of `assert_precondition` in downstream Chromium tests.
* As it is not being renamed, the 'PRECONDITION\_FAILED' status may confuse
consumers of WPT results.